feat: add parsig db by varex83 · Pull Request #278 · NethermindEth/pluto

varex83 · 2026-03-12T15:18:04Z

Closes #274

This PR adds two new dependencies:

dyn-clone - allows cloning trait objects such as SignedData
dyn-eq - allows comparing trait objects such as SignedData

They make it possible to clone and compare Box<dyn SignedData> directly. Using them, rather than adding custom clone/equality methods and plumbing for each type, saved a lot of development time.

I’d also like to request allowing dyn-eq in cargo deny. It can throw the errors due to the license (MPL)

crates/core/src/types.rs

varex83 · 2026-03-19T10:01:06Z

@claude

claude · 2026-03-19T10:01:20Z

Claude finished @varex83's task in 3m 54s —— View job

Code Review

Read changed files
Review signeddata.rs changes
Review types.rs changes
Review parasigdb/ module (memory, metrics, mod)
Review testutils.rs and testutil/random.rs
Review Cargo.toml dependency additions
Summarize findings

Summary

This PR erases the SignedData<Error> associated type in favor of a fixed SignedDataError, makes ParSignedData/ParSignedDataSet type-erased via Box<dyn SignedData>, and wires up a new in-memory ParSigDB. The core design is sound, but there are a few issues worth addressing before merge.

Findings

[High] `keys_by_duty` accumulates duplicate entries on every store

Impact: Unbounded memory growth proportional to the number of partial signatures received per key (not per unique key). After N signatures for the same (duty, pubkey), keys_by_duty[duty] contains N copies of the same Key. During trim, entries.remove(&key) is called N times (only the first has any effect). On a busy validator, this could grow large.

Evidence: crates/core/src/parasigdb/memory.rs:342-352

inner.entries.entry(k.clone()).or_insert_with(Vec::new).push(value.clone());
inner.keys_by_duty.entry(k.duty.clone()).or_insert_with(Vec::new).push(k.clone());  // ← always pushed

Recommendation: Only push to keys_by_duty when entries[k] was previously empty (i.e., first signature for that key):

let slot = inner.entries.entry(k.clone()).or_insert_with(Vec::new);
if slot.is_empty() {
    inner.keys_by_duty.entry(k.duty.clone()).or_insert_with(Vec::new).push(k.clone());
}
slot.push(value.clone());

Fix this →

[High] `as u64` casts violate `cast_lossless = "deny"` workspace lint

Impact: Will cause cargo clippy to fail.

Evidence: crates/core/src/parasigdb/memory.rs:369,375,395

if (sigs.len() as u64) < threshold {        // line 369
if sigs.len() as u64 == threshold {          // line 375
if set.len() as u64 == threshold {           // line 395

The workspace Cargo.toml:123 has cast_lossless = "deny". usize as u64 is a lossless cast and Clippy will reject it. The rest of the codebase uses u64::try_from(...).expect("...") (see memory_internal_test.rs:23-26).

Recommendation: Replace with u64::try_from(sigs.len()).unwrap_or(u64::MAX) (or .expect("len fits in u64") since this is production code and unwrap is allowed in tests only per clippy.toml:1).

Fix this →

[Medium] `SignedDataError::Custom` uses non-`Send` inner type

Impact: Any async future that propagates a Custom error through ? will not be Send, causing compile errors when spawning tasks. MemDB::store_external calls get_threshold_matching which returns Result<_, MemDBError> and MemDBError wraps SignedDataError. If Custom is ever returned in that path, tokio::spawn calls wrapping store_external will fail to compile.

Evidence: crates/core/src/signeddata.rs:53

Custom(Box<dyn std::error::Error>),  // not Send + Sync

Recommendation: Change to Box<dyn std::error::Error + Send + Sync>.

Fix this →

[Medium] Threshold logic requires exactly `threshold` matching signatures

Impact: If threshold + 1 or more identical signatures are collected simultaneously (e.g., in a catch-up scenario), get_threshold_matching returns None and the threshold callback is never triggered. Confirmed by the test case "all identical above threshold" → Vec::new().

Evidence: crates/core/src/parasigdb/memory.rs:394-398

for set in sigs_by_msg_root.values() {
    if set.len() as u64 == threshold {   // ← requires exact match, not >=
        return Ok(Some(set.clone()));
    }
}

Recommendation: Verify this matches Go's parsigmem.go behavior. If Go uses >= instead of ==, this is a parity gap. If intentional, a comment explaining the rationale (e.g., "prevents double-firing of threshold callbacks") would help.

[Low] Reviewer comment from `iamquang95` may be partially resolved

The review comment "can derive directly" on types.rs likely refers to ParSignedData's manual Clone, PartialEq, and Eq impls. These are necessary because Box<dyn SignedData> cannot auto-derive them — the manual impls are correct. The related ParSignedDataSet already uses #[derive(..., Default)]. No action needed, but worth acknowledging the reviewer so they know it was considered.

[Low] `parasigdb` module lacks a descriptive doc comment

Evidence: crates/core/src/lib.rs:27

/// parasigdb
pub mod parasigdb;

Recommendation: Expand to something like: /// Partial signature database for collecting and threshold-checking partial BLS signatures.

[Low] Metrics only tracks exits

Evidence: crates/core/src/parasigdb/metrics.rs — only exit_total is tracked.

The Go implementation tracks additional metrics (total entries, threshold reached counts, etc.). This is a partial implementation; please add a TODO comment or track in a follow-up issue if other metrics are planned.

Parity Check

Component	Go	Rust	Match	Notes
`store_internal`	present	present	yes
`store_external`	present	present	yes
`trim` (cleanup loop)	present	present	yes
Duplicate share_idx detection	present	present	yes
Mismatch error	present	present	yes
Threshold matching (exact vs >=)	TBD	exact	?	Needs Go ref to confirm
`keys_by_duty` dedup	yes (Go uses a Set)	no	no	Bug: see finding #1

Tests

Tests in memory_internal_test.rs cover: threshold matching with multiple variants, full lifecycle with deadliner expiry, and both SyncCommitteeMessage and BeaconCommitteeSelection types. Good coverage overall.

mskrzypkows · 2026-03-19T15:28:09Z

Shouldn't it be parsigdb (without addional 'a')?

Cargo.toml

crates/core/src/testutils.rs

crates/core/src/parsigdb/metrics.rs

iamquang95 · 2026-03-30T11:12:50Z

crates/core/src/types.rs

+pub struct ParSignedData {
    /// Partially signed duty data.
-    pub signed_data: T,
+    pub signed_data: Box<dyn SignedData>,


There is one usages in charon, which downcast the signed_data, storing Box<dyn SignedData> could not downcast this.
attVidx, ok := parSig.SignedData.(core.VersionedAttestation)

Could you share what was discussed during daily @varex83 ?

It was fixed in the #291 by using std::any::Any with downcast_ref to serialize / deserialize objects

emlautarom1

LGTM, minor stuff only

emlautarom1 · 2026-04-02T21:32:06Z

crates/testutil/src/random.rs

+pub fn random_eth2_signature() -> String {
+    let mut bytes = [0u8; 96];
+    let mut rng = rand::thread_rng();
+    for byte in &mut bytes {
+        *byte = rng.r#gen();
+    }
+    format!("0x{}", hex::encode(bytes))
+}


nit: why not reuse the code from random_eth2_signature_bytes?

Suggested change

pub fn random_eth2_signature() -> String {

let mut bytes = [0u8; 96];

let mut rng = rand::thread_rng();

for byte in &mut bytes {

*byte = rng.r#gen();

}

format!("0x{}", hex::encode(bytes))

}

pub fn random_eth2_signature() -> String {

let bytes = random_eth2_signature_bytes();

format!("0x{}", hex::encode(bytes))

}

Same for random_root

emlautarom1 · 2026-04-02T21:37:36Z

crates/testutil/src/random.rs

+        // Check format
+        assert!(bitlist.starts_with("0x"));
+        // 32 bytes = 64 hex chars + "0x" prefix = 66 total
+        assert_eq!(bitlist.len(), 66);


We should check that it actually results in only 5 bits set

Suggested change

assert_eq!(bitlist.len(), 66);

assert_eq!(bitlist.len(), 66);

// Decode to bytes

let bytes = hex::decode(&bitlist[2..]).unwrap();

// Check that at least 5 bits are set

let bit_count = bytes.iter().map(|b| b.count_ones()).sum::<u32>();

assert_eq!(bit_count, 5);

nit: we could parametrize the test to test with multiple lengths (ex. 0, 5, 50, 256)

emlautarom1 · 2026-04-02T21:52:41Z

crates/core/src/signeddata.rs

+    /// Custom error.
+    #[error("{0}")]
+    Custom(Box<dyn std::error::Error>),


Unused variant

emlautarom1 · 2026-04-02T21:56:44Z

crates/core/src/testutils.rs

+/// The size of a BLS public key in bytes.
+const PK_LEN: usize = 48;


This constant is duplicated in crates/core/src/types.rs. We can reference it instead of duplicating it.

emlautarom1 · 2026-04-02T21:58:14Z

crates/core/src/testutils.rs

+pub fn new_seed_rand() -> impl Rng {
+    let seed = rand::random::<u64>();
+    rand::rngs::StdRng::seed_from_u64(seed)
+}


No need for this code, just use a default random generator.

Note: I remember you mentioning that now Claude is overly strict with porting. This is a clear case of that.

emlautarom1 · 2026-04-02T21:59:00Z

crates/core/src/testutils.rs

+/// This should never happen in practice as we generate exactly 48 bytes.
+pub fn random_core_pub_key_seed<R: Rng>(mut rng: R) -> PubKey {
+    let pubkey = deterministic_pub_key_seed(&mut rng);
+    PubKey::try_from(&pubkey[..]).expect("valid pubkey length")


No need for try_from and expect:

Suggested change

PubKey::try_from(&pubkey[..]).expect("valid pubkey length")

PubKey::from(pubkey)

emlautarom1 · 2026-04-02T22:00:16Z

crates/core/src/testutils.rs

+    // Fill the key with random bytes
+    for byte in &mut key {
+        *byte = seeded_rng.r#gen();
+    }


nit: Use fill_bytes:

Suggested change

// Fill the key with random bytes

for byte in &mut key {

*byte = seeded_rng.r#gen();

}

// Fill the key with random bytes

seeded_rng.fill_bytes(&mut key);

emlautarom1 · 2026-04-02T22:20:53Z

crates/core/src/types.rs

+pub struct ParSignedData {
    /// Partially signed duty data.
-    pub signed_data: T,
+    pub signed_data: Box<dyn SignedData>,


Could you share what was discussed during daily @varex83 ?

iamquang95

LGTM 👍

varex83 added 5 commits March 11, 2026 10:47

feat: add deadline.rs

1430acc

feat: deadline tests

364977e

fix: remove comments

368177c

Merge remote-tracking branch 'origin/main' into bohdan/deadline

f4b6893

wip: add parasig db and some tests

de6dba4

varex83 changed the base branch from main to bohdan/deadline March 12, 2026 15:18

refactor: remove old app/deadline

d71c885

varex83 force-pushed the bohdan/deadline branch from c615454 to d71c885 Compare March 12, 2026 15:19

iamquang95 reviewed Mar 16, 2026

View reviewed changes

crates/core/src/types.rs Outdated Show resolved Hide resolved

varex83 added 6 commits March 17, 2026 12:50

feat: add rust docs

13de57a

feat: add clone box and clone eq

fc85e63

Merge branch 'bohdan/update-signeddata-trait' into bohdan/parasigdb

5767cc1

Merge remote-tracking branch 'origin/main' into bohdan/deadline

e214d6f

Merge branch 'bohdan/deadline' into bohdan/parasigdb

7aced94

feat: finish tests

288f63c

varex83 marked this pull request as ready for review March 18, 2026 16:23

varex83 changed the title ~~feat: [wip] add parasig db~~ feat: add parasig db Mar 18, 2026

varex83 changed the title ~~feat: add parasig db~~ feat: add parsig db Mar 19, 2026

fix: typo

a4c4bb2

iamquang95 reviewed Mar 25, 2026

View reviewed changes

Cargo.toml Show resolved Hide resolved

crates/core/src/testutils.rs Show resolved Hide resolved

crates/core/src/parsigdb/metrics.rs Show resolved Hide resolved

fix: review comments

547f85d

iamquang95 reviewed Mar 30, 2026

View reviewed changes

Base automatically changed from bohdan/deadline to main March 31, 2026 09:26

varex83 added 2 commits March 31, 2026 11:35

Merge remote-tracking branch 'origin/main' into bohdan/parasigdb

402fbac

fix: build test issues

19e0a30

emlautarom1 approved these changes Apr 2, 2026

View reviewed changes

fix: review comments

067f365

fix: linter

7328279

iamquang95 approved these changes Apr 3, 2026

View reviewed changes

varex83 merged commit 95cc247 into main Apr 3, 2026
9 checks passed

varex83 deleted the bohdan/parasigdb branch April 3, 2026 15:47

-        assert_eq!(bitlist.len(), 66);
+        assert_eq!(bitlist.len(), 66);
+        // Decode to bytes
+        let bytes = hex::decode(&bitlist[2..]).unwrap();
+        // Check that at least 5 bits are set
+        let bit_count = bytes.iter().map(|b| b.count_ones()).sum::<u32>();
+        assert_eq!(bit_count, 5);

		/// The size of a BLS public key in bytes.
		const PK_LEN: usize = 48;

	PubKey::try_from(&pubkey[..]).expect("valid pubkey length")
	PubKey::from(pubkey)

Conversation

varex83 commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

varex83 commented Mar 19, 2026

Uh oh!

claude bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Summary

Findings

[High] keys_by_duty accumulates duplicate entries on every store

[High] as u64 casts violate cast_lossless = "deny" workspace lint

[Medium] SignedDataError::Custom uses non-Send inner type

[Medium] Threshold logic requires exactly threshold matching signatures

[Low] Reviewer comment from iamquang95 may be partially resolved

[Low] parasigdb module lacks a descriptive doc comment

[Low] Metrics only tracks exits

Parity Check

Tests

Uh oh!

mskrzypkows commented Mar 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlautarom1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamquang95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

varex83 commented Mar 12, 2026 •

edited

Loading

claude bot commented Mar 19, 2026 •

edited

Loading

[High] `keys_by_duty` accumulates duplicate entries on every store

[High] `as u64` casts violate `cast_lossless = "deny"` workspace lint

[Medium] `SignedDataError::Custom` uses non-`Send` inner type

[Medium] Threshold logic requires exactly `threshold` matching signatures

[Low] Reviewer comment from `iamquang95` may be partially resolved

[Low] `parasigdb` module lacks a descriptive doc comment